Over in addCriteriaToChecklist do you want to look for an existing deleted matching criteria and restore it if found? Would that make the distinction between adding and re-adding unnecessary?

An alternative might be to permanently delete the criteria instance once the toast disappears.

I think permanently deleting is probably for the best with this approach. Though see #10165 (comment) for some thoughts on a slightly different approach that I think would lessen the importance of it (would still ultimately be good to have, though).

Just for clarity, we are now permanently deleting the criteria after the toast goes away. I also wanted to add since this is related to the max problem that @eanders-ms expressed above, I removed the check for whether a criteria is deleted on the CatalogOverlay. I did so because having the check would allow a user to add and also readd a criteria, thus possibly having two instances of a criteria that is supposed to only have one. Getting rid of the check in the overlay makes it so it still looks like I can't add the "one only" criteria until the criteria is permanently deleted, thus avoiding the ability for a user to add and readd a criteria that can only be used once.

nit: I saw this as a typo of "read" instead of "re-add". Perhaps "reAdd" or "undelete" would be clearer?

As much as I like the sleekness of this design, something to keep in mind is from an accessibility perspective, how can a user navigate to this button without a mouse? If this means we need to treat toasts like modals and have them steal focus, that could be somewhat confusing/irritating. Maybe we could do that only if the toast is "interactive" in some way? Or there may be other workarounds.

Not necessarily a blocker, but good to keep it in mind.

Good point. I wonder what Outlook does here.

Is removeCriteria the correct tick to send in this case?

So since we're just setting this flag but never removing the criteria, it this means that when a teacher exports their checklist, it will contain all the deleted checklist items which is somewhat wasteful since there's no way to get them back after the toast is dismissed.

One way to avoid this, which I think would ultimately be simpler all around, would be to move criteria out of the main checklist and into a separate deletedCriteria list, rather than add a delete flag. This means that, even if they export while the toast is open, the export will only contain the un-deleted criteria. It also means we don't need all the deleted checks within the code (which would be easy to forget when implementing future features).

Also, if we do a separate list, it's probably still good to remove items once the toast is dismissed, but not quite as important (assuming it clears when the user closes the session).

A downside to a separate list for deleted items is that undoing the deletion wouldn't preserve original location in the active criteria list (unless we stored the original index). If undeletion appended to the active set, it may confuse the user, especially if the end of the list is off the page.

Hmm, that's true, but I still think it'd be simpler to store a separate list of { criteria + index } objects than have to filter based on deleted everywhere (or remember to call the appropriate helper function)

A helper to get active criteria should mitigate this. I agree we don't want to include deleted criteria in the exported JSON.

Even with a helper, you have to remember to call it instead of getting the value from state :) Separate list makes it a non-issue as long as we can sort out the index thing. I don't feel that strongly, so I'm happy for Sarah to decide. But personally, I still think a separate list would be slightly cleaner.

I would like to continue with the flag approach and use the "get active criteria" helper function. I have been thinking A LOT about the pros and cons of each approach, and I think both have really strong merit. The selling point for me is for the removing and readding scenario of criteria. Both approaches have the same amount of state operations for full removing and newly adding criteria. However, in the new scenario that we are supporting here (removing and readding), the "having a separate list for deleted criteria" would need four state operations along with list operations (two for removing, two for readding) while the flag approach only needs two state operations and no list operations are needed.

That's fine, as long as we can get the helper in place and filter deleted criteria out of the serialization.

For long-term code maintainability, I recommend adding a helper (in state/helpers.ts) for getting active criteria.

For code clarity: Use helper to get active criteria, then render them.

We should probably change these colors based on the type of toast containing them (error, info, etc...). I believe there are different css classes for each type of toast we can reference.

Yeah, I think that's a good idea. Can this be done in a follow-up?

Sure

In makeToast, it looks like you added a parameter for jsx, but it seems like we're not using it here. Was there a specific reason for that?

Yes. I originally just had the <UndoDeleteCriteriaButton/> inside the make toast. However, in order to have clicking the undo button dismiss the toast, I needed to have access to the toast id so I switched to setting the toast's jsx to the button after making the toast. That said, I could see future scenarios where passing in jsx when making the toast would be helpful so I left it in. I can remove it, though, since it's not getting used with this change.

I see. I would probably lean towards removing it in that case, at least for now. Either way is fine though.

Hmm, I'm wondering if there's a scenario where we would want this action to work even for non-deleted criteria (in which case we could have a separate action removeDeletedCriteriaFromChecklist that checks for deleted before running remove, or just an onlyIfDeleted parameter passed into this action). But I can't think of a realistic scenario at the moment so maybe it's fine...

I think the c.instanceId === criteriaInstanceId check is redundant. Could just be c.instanceId !== criteriaInstanceId || !c.deleted

I think we should still ensure deleted criteria get filtered out of export/serialization. It's not as big of an issue since it gets removed once the toast goes away, but the user could still export while the toast is around (or more likely, close the browser/tab, so it gets serialized and stored in browser storage), at which point the deleted criteria would be there forever since the delete code won't run on it anymore.

My original comment on this is out of date now, but just a note that I think we need to update this tick from Remove to Re-Add.